Phase 2 , getting Apache 5 compilation and Junit ready along with clearing Checkstyles and spotbug issues #6100
Conversation
…aring Checkstyles and spotbug issues
...ts/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java
Outdated
Show resolved
Hide resolved
| @@ -152,7 +152,9 @@ public void run() { | |||
|
|
|||
| for (Map.Entry<HttpClientConnectionManager, Long> entry : connectionManagers.entrySet()) { | |||
| try { | |||
| entry.getKey().closeIdleConnections(entry.getValue(), TimeUnit.MILLISECONDS); | |||
| entry.getKey().close(CloseMode.GRACEFUL); | |||
There was a problem hiding this comment.
Lets add a todo here to keep track of close_idle_connection_timeout config. We will need to figure out how to support it with Apache5 if we can't do it here.
There was a problem hiding this comment.
Added JAVA-8272 will handle all IM issues in upcoming PR before starting working on test suites
...-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java
Outdated
Show resolved
Hide resolved
| @@ -250,20 +250,28 @@ public void abort() { | |||
| public void close() { | |||
| HttpClientConnectionManager cm = httpClient.getHttpClientConnectionManager(); | |||
| IdleConnectionReaper.getInstance().deregisterConnectionManager(cm); | |||
| cm.shutdown(); | |||
| // TODO : need to add test cases for this | |||
| cm.close(CloseMode.IMMEDIATE); | |||
There was a problem hiding this comment.
Just curious, what are the other options?
There was a problem hiding this comment.
public enum CloseMode {
IMMEDIATE, GRACEFUL
}
| // TODO : This is required since Apache5 closes streams immediately, check memory impacts because of this. | ||
| if (response.getEntity() != null) { | ||
| response.setEntity(new BufferedHttpEntity(response.getEntity())); |
There was a problem hiding this comment.
Hmm, this seems odd. We should not buffer the entire stream.
There was a problem hiding this comment.
Yeap I agree , but as mentioned in
The use of an HTTP response handler guarantees that the underlying HTTP connection will be released back to the connection manager automatically in all cases.
https://hc.apache.org/httpcomponents-client-5.4.x/examples.html
Thus we need to buffer the stream , we need to find a way around this or some better approach.
| @@ -596,12 +606,12 @@ public void setDnsResolver(DnsResolver dnsResolver) { | |||
| } | |||
|
|
|||
| @Override | |||
| public Builder socketFactory(ConnectionSocketFactory socketFactory) { | |||
| public Builder socketFactory(SSLConnectionSocketFactory socketFactory) { | |||
There was a problem hiding this comment.
Is there any reason we are changing this?
There was a problem hiding this comment.
public final PoolingHttpClientConnectionManagerBuilder setSSLSocketFactory(
final org.apache.hc.client5.http.socket.LayeredConnectionSocketFactory sslSocketFactory) {
this.tlsSocketStrategy = (socket, target, port, attachment, context) ->
(SSLSocket) sslSocketFactory.createLayeredSocket(socket, target, port, context);
return this;
}
PoolingHttpClientConnectionManagerBuilder only accepts LayeredConnectionSocketFactory thus we need to do it.
| if (!firstAttempt && isRepeatable()) { | ||
| content.reset(); | ||
| if (!firstAttempt && isRepeatableStream()) { | ||
| getContent().reset(); |
There was a problem hiding this comment.
Is there any reason we are calling getContent() instead of saving it as a member variable? ContentStreamProvider#newStream could be very expensive for some custom implementations.
This PR is continuation of migrating the Apache 4 client
Motivation and Context
Modifications
Testing
License